🎨 Refactor NAComputation with concrete base classes for every operation and ouput new .naviz format
#846
Conversation
ystade
added
enhancement
ystade
force-pushed
the
refactor-na-computation
branch
from
ebe8ff1 to
ba95268
Compare
burgholzer
changed the title
mqt-naviz' formatmqt-naviz format
There was a problem hiding this comment.
Many thanks @ystade for creating this PR.
These are really some great improvements to the usability of the NA components of MQT Core.
As this kind of sets a precedent for changes we might also want to apply to the qc::Operation and qc::QuantumComputation hierarchy. I went through the code with some extra scrutiny.
You will find most of that in the inline comments.
Some of these are really up for discussion, but I think it is worth having that discussion.
I have two more general points:
- Please update the PR title to better reflect the changes in this PR. This is fundamentally refactoring how we handle NA computations and not simply updating an output format. Probably also worth to update the PR description
- Since a couple PRs back, we also have C++ documentation as part of our docs. As such, it would be really great to also have proper C++ documentation for all newly added C++ code. In times of LLM-assistance, I hope this should be moderately easy.
Many thanks for your work on this 🙏🏼
ystade
force-pushed
the
refactor-na-computation
branch
from
7c681ce to
4edea4a
Compare
|
@burgholzer The tests should run through now and all your feedback should be integrated to the best of my knowledge. |
|
Great! Many thanks for addressing this so quickly 🙏🏼 |
|
@burgholzer There is (at least) one issue remaining. I used the convention to append a |
Hm. Probably the cleanest solution would be to adjust the clang-tidy config so that this is not a violation anymore. |
Apparently, there is a specific key for that option. See the changes from the last commit. |
I am pretty sure that is not what we want, as this enforces the trailing underscore. And I am also pretty sure you do not have the time resources to go through all of MQT Core and change that. |
ystade
changed the title
mqt-naviz formatNAComputation with concrete base classes for every operation and ouput new .naviz format
…lation (#849) ## Description This pull request adds support for handling empty quantum and classical registers during the translation process from Qiskit to MQT. This update ensures proper translation and eliminates any potential errors caused by empty registers. This has come up while working on cda-tum/mqt-ddsim#336 ## Checklist: - [x] The pull request only contains commits that are related to it. - [x] I have added appropriate tests and documentation. - [x] I have made sure that all CI jobs on GitHub pass. - [x] The pull request introduces no new warnings and follows the project's style guidelines. Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
There was a problem hiding this comment.
Many thanks for working through all the comments.
This is a really clean PR. I like that very much 😌
I just put on some finishing touches in the last couple of commits. Specifically:
- removed two redundant files that were left over from before the last iteration
- added
@filedocstrings to all new files. (important for getting proper doxygen output) - completed some missing docstrings
- shortened some existing ones where a one-line brief was sufficient
- adjusted the spacing in headers so that the files don't look so cluttered.
- applied some left-over suggestions in the NA computation.
Once CI is green, I will go ahead and merge this.
I'll also create a new 3.0 Beta release so that it becomes easier to directly rely on this in QMAP.
Description
The tool
mqt-navizpurposefully introduces a new textual format that allows to execute operations at a specific time stamp. The current output format of NA computation is not really compatible even thoughmqt-navizsupports a legacy feature to read in this old format with some caveats.This issue triggered a wider refactoring of the
NAComputation. Its entire structure is refactored. Most importantly, concrete Operation have their own class representation inheriting from some super class. At the end, all operations inherit from the classOp. As part of the refactoring, there is a straight forward way to test whether an operation is of some particular type and then it can be casted to this type.Checklist: